-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
exp init: add simple dvc exp init command #6621
Conversation
c93df39
to
e795838
Compare
metrics_no_cache=[metrics], | ||
plots_no_cache=[plots], | ||
live=dvclive, | ||
force=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think force
should probably be an included flag. Not sure it's a good idea to force by default.
Edit: It might be unclear to a new user that this may overwrite an existing stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a note that we obviously need to revisit this later to generalize for other templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add the force
option internally on the next PR. :)
_, ext = os.path.splitext(params_path) | ||
params = list(LOADERS[ext](params_path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't need to block this PR, but see #6605 for proposal to eliminate need for parameter parsing here.
help="Prompt for values that are not provided", | ||
) | ||
experiments_init_parser.add_argument( | ||
"--template", help="Stage template to use to fill with provided values" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should include the default value in the help message (and for all non-boolean options below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add defaults in the upcoming PRs, as the place where I'll keep those defaults may constantly change. :)
params=[{params_path: params}], | ||
metrics_no_cache=[metrics], | ||
plots_no_cache=[plots], | ||
live=dvclive, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default stage can ignore --dvclive
arg. We probably need conditional arg parsing based on the template, throwing an error if a provided argument is irrelevant for that template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have a default stage/template yet, so I left it with the default. I'll change this when we have templates.
Co-authored-by: Dave Berenbaum <[email protected]>
@dberenbaum, I am merging this and will follow up on your comments in the future PRs. The command is hidden meanwhile. |
"--template", help="Stage template to use to fill with provided values" | ||
) | ||
experiments_init_parser.add_argument( | ||
"--explicit", help="Only use the path values explicitly provided" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, feel free to suggest a better name for this option. Maybe --ignore-missing
?
Simpler, initial implementation for
exp init
.Closes #6441.
Part of #6446.